feat(autocomplete): turn on new autocompleter by default MONGOSH-2885 #2549
Conversation
There was a problem hiding this comment.
Pull Request Overview
Enable the new autocompleter by default (unless explicitly disabled via USE_NEW_AUTOCOMPLETE='0') and adjust test expectations accordingly. Some tests were globally skipped, and timing instrumentation was effectively disabled.
- Default condition for enabling new autocompleter changed to env var !== '0'
- Several test suites/contexts were skipped and test logic updated for new enablement semantics
- Startup timing markTime implementation replaced with a no-op (previous logic commented out)
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli-repl/src/startup-timing.ts | Replaced timing capture with no-op, leaving prior implementation commented |
| packages/cli-repl/src/mongosh-repl.ts | Switched feature flag condition to enable new autocompleter unless set to '0' |
| packages/cli-repl/src/mongosh-repl.spec.ts | Updated test conditions to reflect new default enablement logic |
| packages/cli-repl/src/cli-repl.spec.ts | Adjusted autocomplete test logic and skipped a CTRL-C context |
| packages/cli-repl/src/async-repl.spec.ts | Entire AsyncRepl test suite skipped |
| packages/browser-runtime-core/src/autocompleter/shell-api-autocompleter.ts | Updated condition to initialize new autocompleter by default |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| let oldMongoshCompleter: (line: string) => Promise<CompletionResults>; | ||
|
|
||
| if (process.env.USE_NEW_AUTOCOMPLETE) { | ||
| if (process.env.USE_NEW_AUTOCOMPLETE !== '0') { |
There was a problem hiding this comment.
[nitpick] The magic string comparison !== '0' is repeated across multiple files; consider introducing a helper (e.g., function isNewAutocompleteEnabled(env = process.env) { return env.USE_NEW_AUTOCOMPLETE !== '0'; }) to centralize the flag semantics and avoid divergence.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
This is not a horrible idea. If we had it centralized like that, it'd have made it easier to flip the flag.
There was a problem hiding this comment.
Yeah, we can still do this if you prefer? I think this is something that would primarily have made sense when we first introduced the flag
No description provided.